Skip to content
This repository has been archived by the owner on Aug 4, 2019. It is now read-only.

Chuongv/audio tests and demo #52

Merged
merged 15 commits into from
Jun 30, 2015
Merged

Chuongv/audio tests and demo #52

merged 15 commits into from
Jun 30, 2015

Conversation

Chuongv
Copy link
Member

@Chuongv Chuongv commented Jun 27, 2015

Demo has been implemented. Feel free to test it out 😄

Thanks

@dvor
Copy link
Member

dvor commented Jun 27, 2015

This test is failing sometimes. Maybe there is something race condition and toxAV is getting freed too early. Could you check that please? If nothing comes to your head you can simply add logging in test, so next time we would see what's happening there.

Failures:
  0) -[OCTToxAVTests testVersionMethods] (objcToxTests.xctest)
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
/Users/travis/build/Antidote-for-Tox/objcTox/objcToxTests/OCTToxAVTests.m:478: ((toxAV.toxAV == cToxAV) is true) failed:
475 {
476     OCTToxAV *toxAV = [(__bridge OCTToxAVTests *)refToSelf toxAV];
477 
478     CCCAssertTrue(toxAV.toxAV == cToxAV);
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
479 }
480 

@Chuongv
Copy link
Member Author

Chuongv commented Jun 27, 2015

Looking into it.. I believe it's from the testStartAndStopMethod. When you dispatch_source_cancel, it does not interrupt any leftover tox_iterate in the queue.

Cancellation prevents any further invocation of the event handler block for the specified dispatch source, but does not interrupt an event handler block that is already in progress

toxav goes to nil in the dealloc and then tox_iterate runs. Not sure what the best way to work around this is? We can add a cancel completion block to the dispatch_source?

refToSelf needs to be set to NULL after deallocating toxAV.
mocked_tox_iterate checks for the presence of OCTToxAVTests by using
the pointer from refToSelf.
@Chuongv
Copy link
Member Author

Chuongv commented Jun 28, 2015

Okay, I think I solved it now. Was much simpler than I originally thought. Or at least I hope. 😄

@dvor
Copy link
Member

dvor commented Jun 28, 2015

pod install failed for some reason, I've restarted travis.

@dvor
Copy link
Member

dvor commented Jun 28, 2015

Hm, it seems that mannol has deleted his user from github 😮

@Chuongv
Copy link
Member Author

Chuongv commented Jun 28, 2015

Hmm that's not good.... hopefully there's some explanation for this. Right now no one on IRC knows..

@Chuongv
Copy link
Member Author

Chuongv commented Jun 29, 2015

Okay finally got it to pass again 😅 Mannol added the old group audio chats but it was causing problems with the build, so I had to revert it back for now.

@@ -258,6 +260,25 @@ - (void)removeChatWithAllMessages:(OCTChat *)chat
});
}

- (void)removeAllCalls
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to rename method to something more explicit like convertAllCallsToMessages

@Chuongv
Copy link
Member Author

Chuongv commented Jun 30, 2015

Updated, I'm not satisfied with how I did the enums, so feel free to give me any better suggestions

event = OCTMessageCallEventUnanswered;
break;
case OCTCallStatusActive:
event = OCTMessageCallEventAnswered;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add break; here. This may save us from error in future.

case OCTToxAVAudioBitRate16:
newBitrate = OCTToxAVAudioBitRate8;
break;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks okay, you can just change the ordering to be more consistent. From lowest to highest.

@Chuongv
Copy link
Member Author

Chuongv commented Jun 30, 2015

Thanks for the comments, just updated

@dvor
Copy link
Member

dvor commented Jun 30, 2015

With your branches in Antidote-for-Tox project I can see all updates in my feed. So no need to comment after updating now. :-)

dvor added a commit that referenced this pull request Jun 30, 2015
@dvor dvor merged commit 71d2564 into audio Jun 30, 2015
@Chuongv Chuongv deleted the chuongv/audioTestsAndDemo branch June 30, 2015 20:25
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants